Fix children[] array not deleted in deleteNodeRecurs function. #288
Conversation
xinyiz
force-pushed
the
fix-deleteNodeRecurs
branch
from
4a2e019 to
c84c455
Compare
xinyiz
force-pushed
the
fix-deleteNodeRecurs
branch
from
c84c455 to
76cb6ec
Compare
|
Hi, this seems straightforward :) Is something missing to do merge? |
|
Yes, sorry for the delay. I wanted to check and add a few unit test beforehand, but that should happen soon. |
|
We are also running into this assert. Friendly ping :) |
|
@ahornung, Friendly ping :) |
| @@ -705,9 +705,13 @@ namespace octomap { | |||
| // TODO delete check depth, what happens to inner nodes with children? | |||
| this->deleteNodeChild(node, pos); | |||
|
|
|||
| if (!nodeHasChildren(node)) | |||
| if (!nodeHasChildren(node)){ | |||
There was a problem hiding this comment.
Is there a reason not to do this cleanup in deleteNodeChild() already?
It semantically belongs to the deletion of the (last) child, and doing it outside may lead to having to add the same routine to other contexts from where deleteNodeChild is called.
There was a problem hiding this comment.
Good point! The only reason I can think of is the (probably) less efficient call of "pruneNode(...)", which does the cleanup once after deleting all 8 child nodes (instead of checking for every single call of deleteNodeChild if it's empty already).
There was a problem hiding this comment.
True, when you know you are deleting multiple children, you may not want to check after each deletion (which means iterating the 8 child pointers).
So I would suggest adding a parameter to deleteNodeChild() whether to cleanup, which in my opinion should have a default of true.
And if the cleanup is done internally, it might be an idea to return the result of the internal nodeHasChildren() from deleteNodeChild(), saving an extra query as in deleteNodeRecurs()
|
Friendly bump =) |
|
bump! same issue :) |
Fix issue where deleteNode children == __null assertion fails due to children[] array not deleted in deleteNodeRecurs function (https://github.com/OctoMap/octomap/issues/283, https://github.com/OctoMap/octomap/issues/287)